-
Notifications
You must be signed in to change notification settings - Fork 418
offer: make the merkle tree signature public #3892
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
offer: make the merkle tree signature public #3892
Conversation
👋 Thanks for assigning @jkczyz as a reviewer! |
0ffed61
to
be23002
Compare
I'm not personally opposed to this and it may be nice to expose it for the use cases you mention. I'm also not sure about any API guarantees for these methods. I don't think it's too much of a hassle to treat them the same way we treat any other public API. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3892 +/- ##
==========================================
- Coverage 89.00% 88.99% -0.01%
==========================================
Files 166 166
Lines 119990 120093 +103
Branches 119990 120093 +103
==========================================
+ Hits 106798 106878 +80
- Misses 10770 10788 +18
- Partials 2422 2427 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
🔔 1st Reminder Hey @TheBlueMatt @valentinewallace! This PR has been waiting for your review. |
1 similar comment
🔔 1st Reminder Hey @TheBlueMatt @valentinewallace! This PR has been waiting for your review. |
cc @jkczyz |
dd046d5
to
29e5bd6
Compare
Will take a look at this after @jkczyz's comments are addressed, assuming he thinks it needs a second reviewer at that point. |
0c7fe08
to
123e16f
Compare
🔔 2nd Reminder Hey @valentinewallace! This PR has been waiting for your review. |
7429b6a
to
1b958e9
Compare
Ok, I pushed the change that @jkczyz requested, and now the PR is organised in two commits as follows:
Let me know if you are fine with the commit divided in this way, or you prefer e60b87c |
1b958e9
to
699f37f
Compare
22ef851
to
e60b87c
Compare
ec9b1c6
to
98d51cd
Compare
BTW, could you make sure the commit messages are wrapped at 72 characters? |
98d51cd
to
bf5ca05
Compare
Done thanks, should be ready for another pass! |
bf5ca05
to
a68da7e
Compare
/// Returns the [`OfferId`] if this invoice corresponds to an [`Offer`]. | ||
/// | ||
/// [`Offer`]: crate::offers::offer::Offer | ||
pub fn offer_id(&self) -> Option<OfferId> { | ||
self.offer_id | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry... we should put this in invoice_accessors_common
such that UnsignedBolt12Invoice
, UnsignedStaticInvoice
, and StaticInvoice
also get this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I was worried about this accessor in another invoice. Because this change is a little bit disruptive, I made a fixup commit 2da0ac8 to have a first iteration of review on the changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... come to think of it we probably don't need this for the unsigned invoices. Sorry about the hassle. Instead, we can drop the fixup and simply implement it directly for StaticInvoice
. That should never come from a Refund
, so its offer_id
method won't need to use Option
. Thus, we shouldn't use the macro.
2da0ac8
to
a95815c
Compare
lightning/src/offers/invoice.rs
Outdated
let offer_id = match &contents { | ||
InvoiceContents::ForOffer { .. } => Some(OfferId::from_valid_bolt12_tlv_stream(&bytes)), | ||
InvoiceContents::ForRefund { .. } => None, | ||
}; | ||
Self { bytes, experimental_bytes, contents, tagged_hash, offer_id } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkczyz do you think that we should add in OfferId
a maybe_from_valid_bolt12_tlv_stream(&bytes) -> Option<OfferId>
to include the refund case without duplicate the match
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd need to take in the InvoiceContents
then, too. Leaning towards not doing this for unsigned invoices after all. See next comment.
lightning/src/offers/invoice.rs
Outdated
let offer_id = match &contents { | ||
InvoiceContents::ForOffer { .. } => Some(OfferId::from_valid_bolt12_tlv_stream(&bytes)), | ||
InvoiceContents::ForRefund { .. } => None, | ||
}; | ||
Self { bytes, experimental_bytes, contents, tagged_hash, offer_id } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd need to take in the InvoiceContents
then, too. Leaning towards not doing this for unsigned invoices after all. See next comment.
/// Returns the [`OfferId`] if this invoice corresponds to an [`Offer`]. | ||
/// | ||
/// [`Offer`]: crate::offers::offer::Offer | ||
pub fn offer_id(&self) -> Option<OfferId> { | ||
self.offer_id | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... come to think of it we probably don't need this for the unsigned invoices. Sorry about the hassle. Instead, we can drop the fixup and simply implement it directly for StaticInvoice
. That should never come from a Refund
, so its offer_id
method won't need to use Option
. Thus, we shouldn't use the macro.
079db78
to
0e5217d
Compare
Ok pushed the changes, not sure that I have got 100% this review, but let me know if there is something that is still pending #3892 (comment) |
097c4a7
to
8df655b
Compare
I noted that for some reason, I was not making public the |
8df655b
to
5ceaf4d
Compare
let issuer_sign_pubkey = offer.issuer_signing_pubkey().unwrap(); | ||
let tagged_hash = invoice.tagged_hash(); | ||
let signature = invoice.signature(); | ||
assert!(merkle::verify_signature(&signature, tagged_hash, issuer_sign_pubkey).is_ok()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like exposing verify_signature
doesn't provide much value as it's simply a wrapper around the secp256k1
crate. Maybe it is fine to expose as a convenience function, but it isn't strictly necessary to support the use case (i.e., all we need to do is make tagged_hash
accessible).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it, from an API point of view I think it is nice to have a verify_signature
, but I am not oppose to use secp256k1
5ceaf4d
to
dde9e32
Compare
This is helpfull for the users that want to use the merkle tree signature in their own code, for example to verify the signature of bolt12 invoices or recreate it. Very useful for people that are building command line tools for the bolt12 offers. Signed-off-by: Vincenzo Palazzo <[email protected]>
dde9e32
to
c5fe6a3
Compare
- Add an Option<OfferId> field to Bolt12Invoice to track the originating offer. - Compute the offer_id for invoices created from offers by extracting the offer TLV records and hashing them with the correct tag. - Expose a public offer_id() accessor on invoice. - Add tests to ensure the offer_id in the invoice matches the originating Offer, and that refund invoices have no offer_id. - All existing and new tests pass. This enables linking invoices to their originating offers in a robust and spec-compliant way. Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com> Signed-off-by: Vincenzo Palazzo <[email protected]>
c5fe6a3
to
f5fbd6c
Compare
I had to rebase to make the CI happy because there was a problem fixed by some PR that is now merged. So I think the only missing thing to resolve is the following one #3892 (comment) Would love to have the API to hide the |
} | ||
|
||
#[test] | ||
fn verifies_invoice_signature_with_tagged_hash() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this third test belongs in the first commit. :P
This is helpful for users who want to use the Merkle tree signature in their own code, for example, to verify the signature of bolt12 invoices or recreate it.
Very useful for people who are building command line tools for the bolt12 offers.
I am opening this, but I do not know if it is something that you want to do, but at the same time, IMHO, this is very useful to expose because it allows to use LDK in command line tools and in learning tools.
However, this is not strictly necessary because
Bolt12Invoice::try_from
already verifies the signature, but I would open this to know your point of view.